-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Elasticsearch: Don't throw exception on missing CA cert file #5265
Elasticsearch: Don't throw exception on missing CA cert file #5265
Conversation
The current check throws an exception if the version is supposed to be greater than 8 and the ca cert file is missing. This posed two problems: 1. A custom docker image might have a 'latest' version, and thus is newer than version 8 2. A user may have configured their own CA in a different path In order to accomodate for this, a missing copy of a the ca crt file will log a warning, but not throw an exception and leave the container unstarted. There is also another public method, allowing to read the cert, called `extractCert`. Closes testcontainers#5264
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the prompt investigation and fix @spinscale, much appreciated 🙌
I added a comment regarding the API.
...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Only thing I was wondering if we should maybe also add a test utilizing withCertPath
if it is not too involved.
Hm, I could copy another http_ca.crt into the container, but that would need to be done before Elasticsearch starts, is this possible? |
|
oooh... of course. thx. I took a small shortcut and added a http_ca.crt file, that will not match the one created by Elasticsearch on startup and therefore fails on purpose when trying to connect to Elasticsearch using that for a client with a handshake exception. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please add a test that verifies that #5264 works correctly now?
done, tagged an older image as latest and started the container. If there is a better way, let me know, I stole a little code from |
...elasticsearch/src/test/java/org/testcontainers/elasticsearch/ElasticsearchContainerTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
Merged, thanks a lot for this high-quality PR @spinscale, a pleasure to work with you 🥳 |
Hey guys (cc @spinscale), I'm using tag
I understand that this PR (when released), will not throw this exception anymore but that ES will still not start. I'm starting it with the following code:
As you can see, I've tried to turn off security so I'm not sure why it doesn't work in Also, I'd be interested to understand how I can start ES 8.2.0 with TC in a secure mode. I've tried to read https://www.elastic.co/guide/en/elasticsearch/reference/current/docker.html but I'm not sure I understand everything. Am I supposed to provide the Thx! |
Can you try #5264 (comment) as a workaround until the next version is released? |
@spinscale that works fine, thx! |
I confirm it's now working OOB with TC 1.17.2. Thanks for that! However, I still get a warning:
I have turned security off and thus wasn't expecting any warning. Is that something that could be improved? Should I create an issue for it @spinscale ?
Thx |
The current check throws an exception if the version is supposed to be
greater than 8 and the ca cert file is missing. This posed two problems:
newer than version 8
In order to accomodate for this, a missing copy of a the ca crt file
will log a warning, but not throw an exception and leave the container
unstarted. There is also another public method, allowing to read the
cert, called
extractCert
.Closes #5264